Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: [DHIS2-13237] Enrollment coordinates in enrollment widget #3141

Merged
merged 61 commits into from
Nov 20, 2023
Merged

feat: [DHIS2-13237] Enrollment coordinates in enrollment widget #3141

merged 61 commits into from
Nov 20, 2023

Conversation

jasminenguyennn
Copy link
Contributor

@jasminenguyennn jasminenguyennn commented Dec 7, 2022 •
edited by jira bot
Loading

DHIS2-13237

Tech summary
AddLocation.component.js

  1. toggle the MapModal
  2. show either Add Coordinates or Add Area based on the geometryType

MiniMap.component

  1. displays the minimap
  2. converts data to client format using convertToClientCoordinates and pass it as defaultValues to MapModal

MapModal.container.js

  1. onSetCoordinates uses the useUpdateEnrollment hook to update the geometry

Coordinates.component.js

  1. displays the map for a point
  2. renderLatitude and renderLongitude display the coordinates of the point under the map
  3. manage the validation of the coordinates using isValidCoordinate
  4. convert the data to API format usingconvertCoordinatesToServer

Polygon.component.js

  1. displays the map for a polygon
  2. convert the data to API format usingconvertPolygonToServer

DeleteControl.component

  1. refactor the logic of the capture-ui component into a functional component
  2. builds and adds to the map a custom delete control button

@jasminenguyennn jasminenguyennn requested a review from a team as a code owner December 7, 2022 13:30
@JoakimSM
Copy link
Member

JoakimSM commented Dec 12, 2022 •
edited
Loading

I see a map, that's good 🙂 Tried to register a new TEI and enroll, but the marker is not placed in the right spot. Maybe a mixup of Latitude and Longitude? (or are we been inconsistent here somehow) Also, we should be able to update or set a new coordinate/polygon somehow, started a thread on slack for this since the design is incomplete it seems.

@github-actions
Copy link

github-actions bot commented Jan 5, 2023 •
edited
Loading

🚀 Deployed on https://deploy-preview-3141--dhis2-capture.netlify.app

@simonadomnisoru
Copy link
Contributor

simonadomnisoru commented Aug 17, 2023 •
edited
Loading

Hey @superskip,

I've rollback the disabled buttons logic and implemented the same design we currently have when the polygon is created/edited/deleted when enrolling a TEI. This fixes the original issue you reported:

  • Click on a MiniMap containing a polygon. Click the edit polygon button and close modal without first clicking 'Save|Cancel'. Click on the MiniMap again. The 'Save|Cancel' buttons are still displayed. The same is also true for the create and delete buttons.

IMHO I would rather merge the enrollment map with the same design as the existing one (just as the Jira ticket description mentions). Afterward, if it needs improvements, we can focus on changing all the maps at the same time, not just the enrollment widget map. Let me know if you strongly disagree or find any difference between how the polygons modal works in both places.

Thank you for the review!

superskip
superskip reviewed Aug 17, 2023
View reviewed changes
Copy link
Contributor

@superskip superskip left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The inviting blue button is definitely evil now.. I don't think this feels very releasable at the moment, even if the profile widget does the same. If we're in a hurry with releasing this, maybe we should make it look like a useless feature like in the profile widget, by not showing the polygon any more as soon as the page gets reloaded.

If we can make a common map modal component for both widgets, then I agree it would make sense to improve both simultaneously. I also imagine much can be done by improving the enrollment widget first, if one focuses on making the component reusable.

This issue wasn't yours from the start, so we might consider passing it on to me for a while if you're tired of working with it 😛

src/core_modules/capture-core/components/WidgetEnrollment/MiniMap/MiniMap.component.js
onUpdate={updateMutation}
enrollment={enrollment}
/>
{isOpenMap && (
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nice solution :)

JoakimSM
JoakimSM reviewed Oct 17, 2023
View reviewed changes
Copy link
Member

@JoakimSM JoakimSM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@superskip gave me an overview of the problems he saw. Based on this I suggest the following:

  • When opening the modal, before any changes have been made, don't display the 'Set area' button at all. We should keep the cancel button, but change the text to 'Close'.
  • When starting to draw an area, display 'Close without saving' (same behaviour as current 'Cancel') and 'Set area'. 'Set area' should be disabled (also show a tooltip, 'finish drawing before saving')
  • After an area has been drawn, display the buttons we have today (but replace 'Cancel' button text with 'Close without saving')
  • Clicking 'cancel' in the drawing should get you back to the previous state (If no changes have been made earlier, we will only display 'Close' for example)
  • Remove the option to edit the current polygon (the button that says "edit layers"). If you need to make changes, you will have to draw a new one. We need a proper design here ( https://dhis2.atlassian.net/browse/DHIS2-15970), so we will implement in different ticket.

Also, we should consolidate the map code at a later point: https://dhis2.atlassian.net/browse/TECH-1660

Let's talk if you have questions / concerns

@JoakimSM JoakimSM removed the joakim label Oct 17, 2023
@simonadomnisoru
Copy link
Contributor

  • When opening the modal, before any changes have been made, don't display the 'Set area' button at all. We should keep the cancel button, but change the text to 'Close'.
  • When starting to draw an area, display 'Close without saving' (same behaviour as current 'Cancel') and 'Set area'. 'Set area' should be disabled (also show a tooltip, 'finish drawing before saving')
  • After an area has been drawn, display the buttons we have today (but replace 'Cancel' button text with 'Close without saving')
  • Clicking 'cancel' in the drawing should get you back to the previous state (If no changes have been made earlier, we will only display 'Close' for example)
  • Remove the option to edit the current polygon (the button that says "edit layers"). If you need to make changes, you will have to draw a new one. We need a proper design here ( https://dhis2.atlassian.net/browse/DHIS2-15970), so we will implement in different ticket.

Also, we should consolidate the map code at a later point: https://dhis2.atlassian.net/browse/TECH-1660

Hey @JoakimSM,
I implemented the changes you suggested in the latest commits. Let me know if you find other issues.
Thanks!

@JoakimSM
Copy link
Member

JoakimSM commented Nov 10, 2023 •
edited
Loading

Much better! Sometimes when I'm deleting (see video), it doesn't actually get deleted. I think the problem always occurs when: First editing an existing area and saving (set area), then clicking the minimap again and deleting (no browser refresh in between).

Skjermopptak.2023-11-10.kl.15.24.49.mov

This is hopefully the last hurdle.

@simonadomnisoru
Copy link
Contributor

Much better! Sometimes when I'm deleting (see video), it doesn't actually get deleted. I think the problem always occurs when: First editing an existing area and saving (set area), then clicking the minimap again and deleting (no browser refresh in between).

Skjermopptak.2023-11-10.kl.15.24.49.mov
This is hopefully the last hurdle.

Hey @JoakimSM,
I fixed the delete bug with the latest commit. Let me know if you find other issues.
Thank you!

JoakimSM
JoakimSM approved these changes Nov 14, 2023
View reviewed changes
Copy link
Member

@JoakimSM JoakimSM left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉

geethaalwan
geethaalwan approved these changes Nov 17, 2023
View reviewed changes
Copy link

@geethaalwan geethaalwan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested successfully on 2.41,2.40.3,2.39.4,2.38.6 versions

@simonadomnisoru simonadomnisoru merged commit 2f2e52c into master Nov 20, 2023
37 checks passed
@simonadomnisoru simonadomnisoru deleted the DHIS2-13237 branch November 20, 2023 10:44
dhis2-bot added a commit that referenced this pull request Nov 20, 2023
@dhis2-bot
chore(release): cut 100.45.0 [skip release]
05d268e 
# [100.45.0](v100.44.7...v100.45.0) (2023-11-20)

### Features

* [DHIS2-13237] Enrollment coordinates in enrollment widget ([#3141](#3141)) ([2f2e52c](2f2e52c))
@dhis2-bot
Copy link
Contributor

🎉 This PR is included in version 100.45.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JoakimSM JoakimSM JoakimSM approved these changes

@geethaalwan geethaalwan geethaalwan approved these changes

@superskip superskip superskip approved these changes

@simonadomnisoru simonadomnisoru Awaiting requested review from simonadomnisoru

Assignees

@simonadomnisoru simonadomnisoru

Labels
released testing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@jasminenguyennn @JoakimSM @simonadomnisoru @dhis2-bot @geethaalwan @superskip